Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: find the chosen okta mfa option in the mfaOptions #1133

Merged
merged 8 commits into from
Sep 29, 2023
Merged

fix: find the chosen okta mfa option in the mfaOptions #1133

merged 8 commits into from
Sep 29, 2023

Conversation

russell
Copy link
Contributor

@russell russell commented Sep 21, 2023 •
edited
Loading

find the index of the a duoMfaOption by looking in the duoMfaOptions.

the duoMfaOptions is dynamically created, so it could have any number of items, the reason that using the interactively selected mechanism works is that it's choosing based on the list of duoMfaOptions options rather than hardcoded ids. This change does something similar, but searches through duoMfaOptions to find the option passed at the CLI, if the item is missing, then an error will be emitted.

for example if I don't have passcode enabled and if i try to force that option

$ ./saml2aws login --session-duration=43200 --mfa='DUO' --duo-mfa-option='Passcode' --skip-prompt --force --role='arn:aws:iam::183823948234:role/okta/team'
Using IdP Account default to access Okta https://foobar.okta.com/app/amazon_aws/aoeusnthoaenuth/sso/saml
Authenticating as [email protected] ...
Error authenticating to IdP.: error verifying MFA: error unsupported MFA option 'Passcode'

fixes #1079 #1061

@russell russell changed the title fix: find the chosen mfa option in the mfaOptions fix: find the chosen okta mfa option in the mfaOptions Sep 21, 2023
@codecov-commenter
Copy link

codecov-commenter commented Sep 22, 2023 •
edited
Loading

Codecov Report

Merging #1133 (eb8dc09) into master (391a961) will increase coverage by 1.81%.
The diff coverage is 66.66%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##           master    #1133      +/-   ##
==========================================
+ Coverage   37.19%   39.00%   +1.81%     
==========================================
  Files          53       53              
  Lines        7959     7963       +4     
==========================================
+ Hits         2960     3106     +146     
+ Misses       4619     4442     -177     
- Partials      380      415      +35
Flag Coverage Δ
unittests 39.00% <66.66%> (+1.81%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
pkg/provider/okta/okta.go 38.14% <66.66%> (+11.61%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

mapkon
mapkon suggested changes Sep 26, 2023
View reviewed changes
Copy link
Contributor

@mapkon mapkon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - do you mind adding tests to this? This is going to push the coverage down and make the build to fail

mapkon and others added 3 commits September 27, 2023 13:42
@mapkon
Merge branch 'master' into mfa-options
a396b3f
@russell
fix: find the chosen mfa option in the mfaOptions
d4f6c44
@russell
Merge remote-tracking branch 'origin/mfa-options' into mfa-options
88c032b 
* origin/mfa-options:
  chore(deps): bump github.com/tidwall/gjson from 1.16.0 to 1.17.0
  fix: find the chosen mfa option in the mfaOptions
  chore(deps): bump github.com/aws/aws-sdk-go from 1.45.7 to 1.45.12
@russell
Copy link
Contributor Author

russell commented Sep 27, 2023 •
edited
Loading

Sure! here is a test. Can you give me some feedback about the fixtures, i haven't trimmed them down to only the fields that are referenced in the okta.go calls, if you like i can do that. I left them with more information rather than less in the hope that future readers would be able to improve the logic without needing access to fresh raw responses from duo. But each to their own, would you like them trimmed?

@mapkon
Copy link
Contributor

mapkon commented Sep 27, 2023

Sure! here is a test. Can you give me some feedback about the fixtures, i haven't trimmed them down to only the fields that are referenced in the okta.go calls, if you like i can do that. I left them with more information rather than less in the hope that future readers would be able to improve the logic without needing access to fresh raw responses from duo. But each to their own, would you like them trimmed?

I personally think we should trim them down so that the test is focused on what it is testing. The extra context can be confusing.

@gliptak thoughts?

@russell
Copy link
Contributor Author

russell commented Sep 28, 2023

I personally think we should trim them down so that the test is focused on what it is testing. The extra context can be confusing.

i'm cool with doing that, i'll update the PR later today. I'm a bit on the fence personally so it's not a big deal

@russell
Copy link
Contributor Author

russell commented Sep 28, 2023

ok, this is ready for review again, I have extended the test suite to assert all the server request's and trimmed down the fixtures

@mapkon mapkon merged commit b452ec5 into Versent:master Sep 29, 2023
8 checks passed
@mapkon
Copy link
Contributor

mapkon commented Oct 27, 2023

@russell This has been released

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mapkon mapkon mapkon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

panic: runtime error: index out of range [1] with length 1
3 participants
@russell @codecov-commenter @mapkon